Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Resolved the issue of skipping Deps dependent downloads under CI for Macos #1773

Merged
merged 2 commits into from
Jul 20, 2023

Conversation

Mixficsol
Copy link
Collaborator

@Mixficsol Mixficsol commented Jul 20, 2023

fix: Resolved the issue of skipping Deps dependent downloads under CI for Macos

src/pika.cc Outdated
@@ -32,7 +32,7 @@ extern std::unique_ptr<net::NetworkStatistic> g_network_statistic;
static void version() {
char version[32];
snprintf(version, sizeof(version), "%d.%d.%d", PIKA_MAJOR, PIKA_MINOR, PIKA_PATCH);
std::cout << "-----------Pika server----------" << std::endl;
std::cout << "-----------Pika servr----------" << std::endl;
std::cout << "pika_version: " << version << std::endl;
std::cout << pika_build_git_sha << std::endl;
std::cout << "pika_build_compile_date: " << pika_build_compile_date << std::endl;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the provided code patch, there is a typo in the output message being printed. Instead of "Pika servr," it should be "Pika server." It seems to be a simple typo error.

Improvement suggestion:

  1. Instead of using a fixed-size buffer for the version variable, consider using a safer option like std::string or std::ostringstream to avoid potential buffer overflow issues. Here's an example using std::ostringstream:
#include <sstream>
// ...

static void version() {
  std::ostringstream oss;
  oss << PIKA_MAJOR << '.' << PIKA_MINOR << '.' << PIKA_PATCH;
  std::cout << "-----------Pika server----------" << std::endl;
  std::cout << "pika_version: " << oss.str() << std::endl;
  std::cout << pika_build_git_sha << std::endl;
  std::cout << "pika_build_compile_date: " << pika_build_compile_date << std::endl;
}

This approach eliminates the need for explicitly managing buffer sizes and helps prevent potential bugs related to buffer overflows.

Apart from these suggestions, it's difficult to provide further insights without having access to the context and the rest of the codebase.

@@ -145,7 +145,6 @@ jobs:
key: ${{ runner.os }}-dependencies

- name: install Deps
if: ${{ steps.cache.outputs.cache-hit != 'true' }}
run: |
brew update
brew install autoconf protobuf python llvm wget git
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the entire context and additional details, it's challenging to provide a comprehensive code review. However, based on the provided code patch, here are a few observations:

  1. The if condition for the install Deps step has been removed (if: ${{ steps.cache.outputs.cache-hit != 'true' }}). It appears that this condition relied on the output of a previous step named cache. Without further information, it's uncertain why this condition was removed. Ensure this change aligns with the intended behavior.

Improvement suggestions:

  1. It's generally a good practice to keep comments or documentation in the code. Adding a comment explaining the purpose of the install Deps step could be beneficial for future developers who may work on the codebase.

  2. Consider using a package manager, such as Homebrew, to manage the dependencies rather than installing them individually. This can simplify dependency management, especially in larger projects.

  3. If possible, pinning specific versions of the dependencies can enhance stability and reproducibility. This ensures that everyone working on the project uses the same version of the dependencies.

  4. Depending on your specific use case, you might want to consider providing a fallback plan in case the dependencies fail to install. For example, you can add error-handling logic or alternative installation methods.

Remember, for a complete code review, it's important to understand the entire codebase, its purpose, and any relevant documentation or requirements.

@@ -145,7 +145,6 @@ jobs:
key: ${{ runner.os }}-dependencies

- name: install Deps
if: ${{ steps.cache.outputs.cache-hit != 'true' }}
run: |
brew update
brew install autoconf protobuf python llvm wget git
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the code patch you provided, here are some observations and suggestions for improvement:

  1. The if condition in the "install Deps" step has been commented out (- if: ${{ steps.cache.outputs.cache-hit != 'true' }}). It seems that this condition was intended to check whether the cache was hit during a previous step. If the condition is necessary for correct execution, you should uncomment it.

  2. Consider adding a comment explaining the purpose of the "install Deps" step, especially if it's not immediately obvious from the context.

  3. Check if the dependencies listed in the brew install command (autoconf, protobuf, python, llvm, wget, and git) are necessary for your project. Ensure that these dependencies are compatible with your target environment and version requirements.

  4. It's generally a good practice to specify specific versions or version ranges for the dependencies being installed. This helps ensure reproducible builds and avoids unexpected compatibility issues. For example, you could use brew install autoconf@<version> protobuf@<version> to install specific versions of autoconf and protobuf.

  5. Consider using a package manager specific to your programming language (e.g., pip for Python) to manage language-specific dependencies instead of installing them through Homebrew. This approach allows for better dependency management within the context of your project.

  6. Make sure to have error handling and validation mechanisms in place to handle potential failures during the installation process. You may want to consider adding set -e at the beginning of the script to make it exit immediately if any command returns a non-zero status.

Remember, these suggestions are based solely on the provided code patch. A comprehensive review would require examining the entire codebase, understanding the project's requirements, and considering best practices specific to your coding standards and development guidelines.

@Mixficsol Mixficsol changed the title 【WIP】just test fix: Resolved the issue of skipping Deps dependent downloads under CI for Macos Jul 20, 2023
Copy link
Contributor

@yaoyinnan yaoyinnan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chejinge chejinge merged commit 5e8bf5b into OpenAtomFoundation:unstable Jul 20, 2023
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants